feat(register): [PM-27084] Account Register Uses New Data Types #6715
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6715 +/- ##
==========================================
+ Coverage 55.72% 59.91% +4.18%
==========================================
Files 1949 1965 +16
Lines 86539 87037 +498
Branches 7725 7765 +40
==========================================
+ Hits 48227 52150 +3923
+ Misses 36506 32993 -3513
- Partials 1806 1894 +88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (1)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
…ed up reference to master password hash
…d more comments and fixed up some long lines.
…unts controller no longer nullish allowed.
…e thrown error messages more appropriate
|
Adding @eligrubb to review because he has work he is branching off of this to do! |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs
Outdated
Show resolved
Hide resolved
src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs
Outdated
Show resolved
Hide resolved
src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs
Outdated
Show resolved
Hide resolved
src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs
Outdated
Show resolved
Hide resolved
src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs
Outdated
Show resolved
Hide resolved
…ved unused import.
Patrick-Pimentel-Bitwarden
requested review from
ike-kottlowski,
mzieniukbw and
quexten
| IdentityResult? identityResult = null; | ||
|
|
||
| // PM-28143 - Just use the MasterPasswordAuthenticationData.MasterPasswordAuthenticationHash | ||
| string masterPasswordHash = model.MasterPasswordAuthentication?.MasterPasswordAuthenticationHash |
There was a problem hiding this comment.
Please rename to "masterPasswordAuthenticationHash", or just use the MasterPasswordAuthentication struct all the way through. Even if it is not on the request model, you can still construct it from the email and kdf that are on the request model.
There was a problem hiding this comment.
I don't think I want to reconstruct it right now in the interest of trying to keep things simpler. Is that okay with you to just do the rename?
As an aside, would you mind pointing me to where I would look to see an example of how to reconstruct it in the server so I can learn?
There was a problem hiding this comment.
Sounds reasonable.
What I meant by reconstruction is that the authentication data is just the "MasterPasswordHash", KDF, salt (lower-cased and trimmed email) packed into a struct. You can just directly make it with something like:
MasterPasswordAuthenticationData
{
Kdf = Kdf {...}
MasterPasswordAuthenticationHash = MasterPasswordHash,
Salt = email.toLower().trim()
};
| // Always force a valid encrypted string for tests to avoid model validation failures. | ||
| var masterKeyWrappedUserKey = DefaultEncryptedString; |
There was a problem hiding this comment.
This requires fixing the tests that use this function.
There was a problem hiding this comment.
Yes I agree it shouldn't override. However the tests will be addressed in PM-28143 and will have to be updated to use the new data types then.
…ed validation tests and ToUser no longer throws bad request.
…ressed feedback and added tests.
…epts-new-data-types
…ressed more feedback. No longer overriding the master password hash.
Patrick-Pimentel-Bitwarden
deleted the
auth/pm-27084/register-accepts-new-data-types
branch
Patrick-Pimentel-Bitwarden
restored the
auth/pm-27084/register-accepts-new-data-types
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27084
📔 Objective
📸 Screenshots
Screen.Recording.2025-12-11.at.5.21.28.PM.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes